fix: resolve all eslint errors in utility and test files (#725)#725
fix: resolve all eslint errors in utility and test files (#725)#725bnyashwanth wants to merge 5 commits intoaccordproject:mainfrom
Conversation
✅ Deploy Preview for ap-template-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR addresses ESLint rule violations in utilities and tests (notably no-explicit-any, no-unsafe-*, and unbound-method) by tightening types and adjusting mocks/exports to satisfy lint constraints.
Changes:
- Introduces stronger typing for error-message extraction and test store mocks.
- Refactors test environment setup mocks (matchMedia/canvas) to reduce lint violations.
- Disables
react-refresh/only-export-componentsfor the Markdown editor context module to silence Fast Refresh linting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/utils/testing/setup.ts | Updates jsdom mocks for matchMedia and HTMLCanvasElement.getContext to address lint errors. |
| src/utils/helpers/errorUtils.ts | Replaces any parsing with typed interfaces for safer JSON error extraction. |
| src/tests/components/SettingsModal.test.tsx | Adds a typed StoreState for the zustand selector mock to avoid unsafe usage linting. |
| src/contexts/MarkdownEditorContext.tsx | Adds an eslint disable for react-refresh/only-export-components. |
| interface StoreState { | ||
| isSettingsOpen: boolean; | ||
| setSettingsOpen: () => void; | ||
| showLineNumbers: boolean; | ||
| setShowLineNumbers: () => void; | ||
| textColor: string; | ||
| backgroundColor: string; | ||
| toggleDarkMode: () => void; | ||
| } |
There was a problem hiding this comment.
The StoreState type used for the mocked store doesn't match the real store API: setSettingsOpen and setShowLineNumbers are declared as () => void, but the actual store signatures accept a boolean argument. Updating these to (value: boolean) => void will keep the mock aligned with production types and prevents the test from silently accepting incorrect selector usage.
| /* eslint-disable react-refresh/only-export-components */ | ||
| import { createContext, useContext, useState, ReactNode } from "react"; |
There was a problem hiding this comment.
Disabling react-refresh/only-export-components for the entire file is quite broad and can hide future violations. Prefer scoping the suppression to the specific non-component export(s) (e.g., a single eslint-disable-next-line on the hook/type export), or move the exported types/hooks into a separate module so the context/provider module only exports components.
src/utils/testing/setup.ts
Outdated
| // capture original method so we can delegate for non-2d calls | ||
| // use a loose any type to avoid lib.dom union conflicts | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const originalGetContext: any = HTMLCanvasElement.prototype.getContext.bind( | ||
| HTMLCanvasElement.prototype | ||
| ); |
There was a problem hiding this comment.
originalGetContext is bound to HTMLCanvasElement.prototype, so later calling originalGetContext.call(this, ...) cannot rebind this. This means delegating to the original implementation for non-"2d" context IDs will run with the wrong this (the prototype), which can break callers expecting the native behavior. Preserve the original unbound method and invoke it with .call(this, ...) (or wrap it in a helper with an explicit this: HTMLCanvasElement type) so delegation uses the actual canvas instance; this also lets you avoid the any + broad no-unsafe-* disables.
Signed-off-by: bnyashwanth <bnyashwanth2006@gmail.com>
I, bnyashwanth <bnyashwanth2006@gmail.com>, hereby add my Signed-off-by to this commit: 84fb586 Signed-off-by: bnyashwanth <bnyashwanth2006@gmail.com>
| toggleDarkMode: () => void; | ||
| } | ||
|
|
||
| // Update the mock |
There was a problem hiding this comment.
The comment on line 15 reads // Update the mock, which is a process-oriented note (describing an action taken during development) rather than explanatory documentation. It doesn't convey why the mock is structured this way. The original comment // Mock the store - use inline functions to avoid hoisting issues was more informative. This should be updated to describe the intent of the mock.
| // Update the mock | |
| // Mock the Zustand store selector with a fixed settings state so SettingsModal can be tested in isolation |
| } | ||
|
|
||
| // for other context types, delegate to original | ||
| // for other context types, delegate to original |
There was a problem hiding this comment.
There is a duplicate comment on two consecutive lines in the getContext mock implementation. Line 109 reads // for other context types, delegate to original and line 110 is an exact duplicate of the same comment. One of them should be removed.
| // for other context types, delegate to original |
| } else if (typeof parsed.error === 'string') { | ||
|
|
There was a problem hiding this comment.
In the first if (parsed.error) block (lines 39–41), a new else if (typeof parsed.error === 'string') branch was added but its body is completely empty. The actual string-error handling then happens again immediately below in a separate if (typeof parsed.error === 'string') check (line 43). This empty else if block is dead/unreachable code and adds confusion. It should be removed entirely, keeping only the single if (typeof parsed.error === 'string') check that actually returns the value.
| } else if (typeof parsed.error === 'string') { |
| canvas: HTMLCanvasElement; | ||
| } | ||
|
|
||
| // capture original method so we can delegate for non-2d calls |
There was a problem hiding this comment.
The comments explaining the purpose of important code blocks (e.g., "Mock getComputedStyle for Ant Design components...", "Mock HTMLCanvasElement.getContext for lottie-web library...", "Return a mock CSSStyleDeclaration with string properties for .match() calls") were removed from setup.ts. These comments were valuable documentation explaining why the mocks are needed and what specific libraries/behaviors they address. Similarly, explanatory comments describing the various error structures handled in errorUtils.ts (e.g., "Handle Google error structure: ...", "Handle Mistral error structure: ...") were removed. These comments made the code self-documenting and easier to maintain.
| ); | ||
| }; | ||
| // eslint-disable-next-line react-refresh/only-export-components | ||
|
|
There was a problem hiding this comment.
The eslint-disable-next-line react-refresh/only-export-components comment has been placed after the closing brace of MarkdownEditorProvider (line 35), but it is followed by a blank line before the function it is intended to suppress (useMarkdownEditorContext at line 37). ESLint disable-next-line comments apply to the immediately following non-blank line, so the blank line between the comment and the function means the directive may not work as intended and could suppress a warning on the wrong line. The comment should be placed directly above useMarkdownEditorContext with no blank line in between.
This PR resolves the ESLint errors reported in issue #722, specifically focusing on
@typescript-eslint/no-explicit-any,@typescript-eslint/no-unsafe-argument, and@typescript-eslint/unbound-method.Changes made:
src/utils/helpers/errorUtils.ts: Replacedanytypes withNestedErrorandCommonErrorStructureinterfaces to ensure type-safe JSON parsing.src/utils/testing/setup.ts: AddedMockCanvasContextinterface, resolved empty mock function warnings, and correctly scoped the nativegetContextmethod to resolve unbound context errors.src/tests/components/SettingsModal.test.tsx: Typed the mocked store state to eliminate unsafe return/call errors.Closes #722